Add TraverseFilter.mapAccumulateFilter #4561

Masynchin · 2024-02-19T16:31:26Z

If Traverse has mapAccumulate, why don't TraverseFilter has mapAccumulateFilter?

Now it has. I implement method itself (almost fully copied from mapAccumulate, changing traverse to traverseFilter), syntax ops, and tests (almost fully copied from mapAccumulate, adjusting fn signature)

johnynek

Thanks for taking the time!

I think if we add this we should make it fast for as many instances in cats that we can.

johnynek · 2024-02-19T16:46:50Z

core/src/main/scala/cats/TraverseFilter.scala

@@ -122,6 +122,9 @@ trait TraverseFilter[F[_]] extends FunctorFilter[F] {
  override def mapFilter[A, B](fa: F[A])(f: A => Option[B]): F[B] =
    traverseFilter[Id, A, B](fa)(f)

+  def mapAccumulateFilter[S, A, B](init: S, fa: F[A])(f: (S, A) => (S, Option[B])): (S, F[B]) =
+    traverseFilter(fa)(a => State(s => f(s, a))).run(init).value


Can we override this for some of the built in collections? State is rather slow so we should avoid it for List, Vector, Chain, NonEmptyList, NonEmptyVector, ...

I can copy StaticMethods.mapAccumulateFromStrictFunctor for mapAccumulateFilter, this will cover for some collections

Masynchin · 2024-02-19T19:31:59Z

There is no override of mapAccumulate for Seq, so should it be added with mapAccumulateFilter through StaticMethods.mapAccumulateFilterFromStrictFunctorFilter?

Masynchin · 2024-02-19T19:35:12Z

There is no override of mapAccumulate for Seq, so should it be added with mapAccumulateFilter through StaticMethods.mapAccumulateFilterFromStrictFunctorFilter?

Same for LazyList and Stream

johnynek

Thanks for sending this PR

armanbilge

Bikeshed question: why mapAccumulateFilter vs mapFilterAccumulate? I think I prefer the latter.

Masynchin · 2024-03-02T13:30:23Z

Bikeshed question: why mapAccumulateFilter vs mapFilterAccumulate?

I just added "Filter"-suffix after original method name from Traverse, same as in traverse → traverseFilter and sequence → sequenceFilter

I have no opinion here, I don't mind both

satorg · 2024-04-05T07:32:12Z

core/src/main/scala/cats/TraverseFilter.scala

@@ -122,6 +122,9 @@ trait TraverseFilter[F[_]] extends FunctorFilter[F] {
  override def mapFilter[A, B](fa: F[A])(f: A => Option[B]): F[B] =
    traverseFilter[Id, A, B](fa)(f)

+  def mapAccumulateFilter[S, A, B](init: S, fa: F[A])(f: (S, A) => (S, Option[B])): (S, F[B]) =


I feel that a scaladoc comment (perhaps, with a short usage example) wouldn't hurt and could come handy here.

Almost done, I should fix the code violations. Is the language correct?

Looks good to me, thanks!

satorg · 2024-04-05T07:39:29Z

There is no override of mapAccumulate for Seq, so should it be added with mapAccumulateFilter through StaticMethods.mapAccumulateFilterFromStrictFunctorFilter?

I think it can be addressed in a separate follow-up PR, just to avoid blowing up the scope of this one.

satorg · 2024-04-05T08:17:12Z

Bikeshed question: why mapAccumulateFilter vs mapFilterAccumulate? I think I prefer the latter.

I just added "Filter"-suffix after original method name from Traverse, same as in traverse → traverseFilter and sequence → sequenceFilter

We just get to decide, whether the new method is likely mapFilter + Accumulate or mapAccumulate + Filter 😄

As a bit more crazy idea (maybe not that crazy though): we can add two functions instead:

def mapAccumulateFilter[S, A, B](init: S, fa: F[A])(f: (S, A) => (S, Option[B])): (S, F[B])
def mapFilterAccumulate[S, A, B](init: S, fa: F[A])(f: (S, A) => Option[(S, B)]): (S, F[B])

The former always does accumulate regardless of filter, hence the name.
The latter always does filter first, then accumulate if not filtered out.

Of course, we can implement mapFilterAccumulate in terms of mapAccumulateFilter, e.g.:

def mapFilterAccumulate[S, A, B](init: S, fa: F[A])(f: (S, A) => Option[(S, B)]): (S, F[B]) =
  mapAccumulateFilter(init, fa) { (s1, a) =>
    f(s1, a) match {
      case (s2, Some(b)) => Some((s2, b))
      case (_, None) => None // drop `S` from this iteration
    }
  }

However both functions can come handy under certain circumstances.
^{(sorry if my snippet is incorrect – it is beyond 1 am already and I am gravily sleepy)}

Moreover, even more important – it could help up to avoid choosing between two really good names and just move on with this PR :)

Masynchin · 2024-04-05T09:12:30Z

Of course, we can implement mapFilterAccumulate in terms of mapAccumulateFilter, e.g.:

def mapFilterAccumulate[S, A, B](init: S, fa: F[A])(f: (S, A) => Option[(S, B)]): (S, F[B]) =
  mapAccumulateFilter(init, fa) { (s1, a) =>
    f(s1, a) match {
      case (s2, Some(b)) => Some((s2, b))
      case (_, None) => None // drop `S` from this iteration
    }
  }

This is implementation of mapAccumulateFilter in terms of mapFilterAccumulate. f pattern matching should be changed inside out:

f(s1, a) match
  case Some((s2, b)) => (s2, Some(b))
  case None          => (s1, None)

satorg · 2024-04-05T18:43:42Z

This is implementation of mapAccumulateFilter in terms of mapFilterAccumulate. f pattern matching should be changed inside out:

Yes, thank you 😊

Anyway, mapFilterAccumulate looks like a special case for mapAccumulateFilter, which is more general. But it can come more handy when accumulation is not needed on entries where the condition does not hold.

@armanbilge – wdyt?

satorg · 2024-04-05T18:51:40Z

tests/shared/src/test/scala/cats/tests/TraverseFilterSuite.scala

@@ -38,6 +38,19 @@ abstract class TraverseFilterSuite[F[_]: TraverseFilter](name: String)(implicit

  implicit def T: Traverse[F] = implicitly[TraverseFilter[F]].traverse

+  test(s"TraverseFilter[$name].mapAccumulateFilter") {


Looks like it does not check the default implementation (based on State), does it?
Except maybe one for Stream, but I wouldn't count on it.

For testing default implementations we usually use ListWrapper from testkit:
ListWrapper.scala

I had straightforwardly copy-pasted and updated tests from mapAccumulate here:

cats/tests/shared/src/test/scala/cats/tests/TraverseSuite.scala

Lines 45 to 56 in 70dbf8f

test(s"Traverse[$name].mapAccumulate") {

forAll { (init: Int, fa: F[Int], fn: ((Int, Int)) => (Int, Int)) =>

val lhs = fa.mapAccumulate(init)((s, a) => fn((s, a)))

val rhs = fa.foldLeft((init, List.empty[Int])) { case ((s1, acc), a) =>

val (s2, b) = fn((s1, a))

(s2, b :: acc)

}

assert(lhs.map(_.toList) === rhs.map(_.reverse))

}

}

If that tests doesn't test default implementation too, I can update mapAccumulateFilter tests. Can you provide an example of how to pass ListWrapper?

For testing default implementations we usually use ListWrapper from testkit: ListWrapper.scala

I am failing to understand how to use ListWrapper to test mapAccumulateFilter. I was looking for examples in other Suites, but it is either suites for data and not typeclasses (OptionT, Try, etc.), or it refers to <Typeclass>Tests[ListWrapper].<methodToTest>, like in the ApplicativeSuite:

cats/tests/shared/src/test/scala/cats/tests/ApplicativeSuite.scala

Lines 102 to 104 in 70dbf8f

implicit val listwrapperApplicative: Applicative[ListWrapper] = ListWrapper.applicative

implicit val listwrapperCoflatMap: CoflatMap[ListWrapper] = Applicative.coflatMap[ListWrapper]

checkAll("Applicative[ListWrapper].coflatMap", CoflatMapTests[ListWrapper].coflatMap[String, String, String])

which I can not apply here, because we don't have TraverseFilter[?].mapAccumulateFilter. Am I missing something?

My apologies for the delay – I was snowed under a bit. Actually, there's TraverseFilter for ListWrapper:

cats/testkit/src/main/scala/cats/tests/ListWrapper.scala

Lines 80 to 90 in 47fbad7

val traverseFilter: TraverseFilter[ListWrapper] = {

val F = TraverseFilter[List]

new TraverseFilter[ListWrapper] {

def traverse = ListWrapper.traverse

def traverseFilter[G[_], A, B](

fa: ListWrapper[A]

)(f: A => G[Option[B]])(implicit G: Applicative[G]): G[ListWrapper[B]] =

G.map(F.traverseFilter(fa.list)(f))(ListWrapper.apply)

}

}

To test the default implementation you can either call it directly:

ListWrapper.traverseFilter.mapAccumulateFilter(...)

or make it an implicit in the scope:

implicit val listWrapperTraverseFilter: TraverseFilter[ListWrapper] = ListWrapper.traverseFilter

And then you can work with TraverseFilter for ListWrapper as usual.

Masynchin added 2 commits February 19, 2024 19:16

Add TraverseFilter.mapAccumulateFilter

021f5f8

Add tests for TraverseFilter.mapAccumulateFilter

17389c0

johnynek reviewed Feb 19, 2024

View reviewed changes

Masynchin added 6 commits February 19, 2024 21:35

Optimize mapAccumulateFilter for List

23fa3db

Optimize mapAccumulateFilter for Vector

8040c23

Optimize mapAccumulateFilter for Chain

cb6d411

Optimize mapAccumulateFilter for Option

277593b

Fix formatting for TraverseFilter[Option].mapAccumulateFilter

15ec803

Optimize mapAccumulateFilter for Queue

9adf4d5

Masynchin added 2 commits February 19, 2024 22:38

Optimize mapAccumulateFilter for ArraySeq

a414be1

Run scalafmt

35706a7

Masynchin requested a review from johnynek February 21, 2024 18:39

johnynek previously approved these changes Feb 22, 2024

View reviewed changes

armanbilge reviewed Mar 2, 2024

View reviewed changes

Masynchin mentioned this pull request Mar 13, 2024

Use mapAccumulateFilter lichess-org/lila-fishnet#287

Closed

satorg reviewed Apr 5, 2024

View reviewed changes

Add scaladoc for mapAccumulateFilter

778aad2

Masynchin dismissed johnynek’s stale review via 778aad2 April 6, 2024 10:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TraverseFilter.mapAccumulateFilter #4561

Add TraverseFilter.mapAccumulateFilter #4561

Masynchin commented Feb 19, 2024

johnynek left a comment

johnynek Feb 19, 2024

Masynchin Feb 19, 2024 •

edited

Loading

Masynchin commented Feb 19, 2024

Masynchin commented Feb 19, 2024 •

edited

Loading

johnynek left a comment

armanbilge left a comment

Masynchin commented Mar 2, 2024 •

edited

Loading

satorg Apr 5, 2024

Masynchin Apr 6, 2024

satorg Apr 10, 2024

satorg commented Apr 5, 2024

satorg commented Apr 5, 2024

Masynchin commented Apr 5, 2024

satorg commented Apr 5, 2024 •

edited

Loading

satorg Apr 5, 2024

Masynchin Apr 5, 2024

Masynchin Apr 6, 2024

satorg Apr 10, 2024 •

edited

Loading

		@@ -38,6 +38,19 @@ abstract class TraverseFilterSuite[F[_]: TraverseFilter](name: String)(implicit

		implicit def T: Traverse[F] = implicitly[TraverseFilter[F]].traverse

		test(s"TraverseFilter[$name].mapAccumulateFilter") {

	test(s"Traverse[$name].mapAccumulate") {
	forAll { (init: Int, fa: F[Int], fn: ((Int, Int)) => (Int, Int)) =>
	val lhs = fa.mapAccumulate(init)((s, a) => fn((s, a)))

	val rhs = fa.foldLeft((init, List.empty[Int])) { case ((s1, acc), a) =>
	val (s2, b) = fn((s1, a))
	(s2, b :: acc)
	}

	assert(lhs.map(_.toList) === rhs.map(_.reverse))
	}
	}

	implicit val listwrapperApplicative: Applicative[ListWrapper] = ListWrapper.applicative
	implicit val listwrapperCoflatMap: CoflatMap[ListWrapper] = Applicative.coflatMap[ListWrapper]
	checkAll("Applicative[ListWrapper].coflatMap", CoflatMapTests[ListWrapper].coflatMap[String, String, String])

	val traverseFilter: TraverseFilter[ListWrapper] = {
	val F = TraverseFilter[List]

	new TraverseFilter[ListWrapper] {
	def traverse = ListWrapper.traverse
	def traverseFilter[G[_], A, B](
	fa: ListWrapper[A]
	)(f: A => G[Option[B]])(implicit G: Applicative[G]): G[ListWrapper[B]] =
	G.map(F.traverseFilter(fa.list)(f))(ListWrapper.apply)
	}
	}

Add TraverseFilter.mapAccumulateFilter #4561

Are you sure you want to change the base?

Add TraverseFilter.mapAccumulateFilter #4561

Conversation

Masynchin commented Feb 19, 2024

johnynek left a comment

Choose a reason for hiding this comment

johnynek Feb 19, 2024

Choose a reason for hiding this comment

Masynchin Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

Masynchin commented Feb 19, 2024

Masynchin commented Feb 19, 2024 • edited Loading

johnynek left a comment

Choose a reason for hiding this comment

armanbilge left a comment

Choose a reason for hiding this comment

Masynchin commented Mar 2, 2024 • edited Loading

satorg Apr 5, 2024

Choose a reason for hiding this comment

Masynchin Apr 6, 2024

Choose a reason for hiding this comment

satorg Apr 10, 2024

Choose a reason for hiding this comment

satorg commented Apr 5, 2024

satorg commented Apr 5, 2024

Masynchin commented Apr 5, 2024

satorg commented Apr 5, 2024 • edited Loading

satorg Apr 5, 2024

Choose a reason for hiding this comment

Masynchin Apr 5, 2024

Choose a reason for hiding this comment

Masynchin Apr 6, 2024

Choose a reason for hiding this comment

satorg Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

Masynchin Feb 19, 2024 •

edited

Loading

Masynchin commented Feb 19, 2024 •

edited

Loading

Masynchin commented Mar 2, 2024 •

edited

Loading

satorg commented Apr 5, 2024 •

edited

Loading

satorg Apr 10, 2024 •

edited

Loading